This could also be removed from here and then replace the assert in line 173 as suggested in a previous PR #15528 (comment)

Depends. The range check should catch it anyway and it provides a message that will be clearer to the user so I'd rather not check it. Removing this and adding an assert would be a better choice.

But I'm not sure if we will actually reach that check with constants. What happens in this case if you remove the isSigned(). Will it fail in constant evaluator instead?

int constant X = -1;
contract C layout at X {}

It still fails the range check and the message is indeed clearer as you said:

Error: The base slot of the storage layout evaluates to -1, which is outside the range of type uint256.

We could also say a rational number or an integer, but not sure if the user will get the distinction. Unfortunately, out terminology here is a bit ambiguous.

This one is weird. Why are we getting this and not something like:

Error: Member "x" not found or not visible after argument-dependent lookup in type(contract A).

Is ConstantEvaluator silencing this error and just returning nullopt?

Yes, ConstantEvaluator can't handle member access. At the moment it can only process, literals, variables with no member acess from modules or other contracts and most of binary/unary operations.

Interesting. So apparently it's not just layouts, but ConstantEvaluator does not support such constants in general. It does not seem hard to add that though. Looking at ConstantEvaluator, it may be as simple as defining endVisit(MemberAccess) similar to endVisit(Identifier) it already has. I think we should do it: #16055.

This should not block this PR though. It's fine to leave it unsupported. You could add an explicit check for MemberAccess to PostTypeContractLevelChecker to issue a better message, but it will only catch cases where it's the top-level expression, so it's a half-measure.

This one should work without errors.

Same for constant_initialized_from_cast.sol

Please also add an immutable that's not pure (i.e. one that's not initialized with a literal) since that's a separate case. Also a comment explaining how they differ.

I think that users will be confused about why we can't evaluate this, because fundamentally it should be possible. It's documented, but maybe it would not hurt to just say that the constant evaluator is just too limited for this?

Done.

I was hoping this would work already in this PR, but apparently it's another thing that requires proper compile-time evaluation... #11183 (comment)

Yeah, the constant evaluator doesn't seem to be evaluating much based on this PR :D

Well, one thing that should work after the PR are simple constants initialized with expressions that we evaluate in unlimited precision. Please make sure that this is the case.

I guess that's the same situation as the test cases in test/libsolidity/syntaxTests/storageLayoutSpecifier/intermediate_operation_out_of_range.sol, right?
I have added a few cases involving constants there.

It is. We should still have at least one dedicated test whose goal is specifically to show that constants work.

I think you're also missing tests with some expression on constants or with constants that refer to other constants. These should all work.

There is test/libsolidity/syntaxTests/storageLayoutSpecifier/constant_initialized_from_other_constant.sol and test/libsolidity/syntaxTests/storageLayoutSpecifier/layout_specification_constant_in_expression.sol which should cover that.

It is. We should still have at least one dedicated test whose goal is specifically to show that constants work.

I separated the constant cases and put them in their own file test/libsolidity/syntaxTests/storageLayoutSpecifier/constant_initialized_with_unlimited_arithmetic_expression.sol.

An integer is a number, so this is a bit redundant. Also, couldn't you reuse error 1763? It's the same error and provides the same source location.

The concern I'd have with reusing error 1763 is that although it has the same location and message (which we could try to improve in both cases to differentiate them), it is a different case where the expression is a rational but it is fractional. In the case here of error 6396 the expression is neither a rational nor an integer number.
We can change the message here to The base slot of the storage layout must evaluate to a number. to clarify and distinguish the errors. What do you think?

I think it's fine to cover both cases with the same error. The exact case is different (fractional literal vs non-integer constant), but conceptually the issue is the same with both - the user is trying to use something that's not an integer and we can't use that as a slot number.

Though, looking at the test cases, perhaps some clarification would be useful in cases where user is trying to use something like address or bytesNN since those can be initialized with an integer.

But that would be a completely different distinction. Meant to show that the type is the issue rather the value.

Should we then change the message here to The base slot type must be a numerical type or something similar?

I think I'd keep it as "must evaluate to an integer" and just do a single check. It's hard to phrase these things.

What I'd really want to do eventually would be to rewrite the docs to introduce clear terminology.

I added a check to append a hint in the cases where the expression is of type address or bytesN.